Improve external-name config of iot thing type #1456
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of your changes
The aws iot ThingType resource uses a parameter called "name" as its terraform id. Unfortunately, when it was first implemented, it was done using IdentifierFromProvider, which means that creating a new managed resource always attempts to create a new external resource, even if there already is one with the same name. For this particular resource, this results in errors from the AWS api.
The best external name config would be
config.NameAsIdentifier
, which would remove the redundantname
property from bothspec.forProvider
andspec.initProvider
, and initialize the name frommetadata.name
. But that's a breaking api change.The next best external name config would be
config.TemplatedStringAsIdentifier("", "{{ .parameters.name }}")
, but that will (correctly) setname
as an identifier field, which will remove it fromspec.initProvider
, which is also a breaking change.So this is the best I can do at the moment without changing the schema. The content of the external name remains the same, but now crossplane knows how to construct it from the
spec
, so it can check for resource existence before trying to create it.Because
name
is part of the id, but not set as an identifier field, various edge cases where the user omits it may continue to fail in less-than-ideal ways, but that's not a change from current behavior.Fixes #
I have:
make reviewable
to ensure this PR is ready for review.backport release-x.y
labels to auto-backport this PR if necessary.How has this code been tested
Running uptest on the pr.
Manually confirming that the external name is the in the same format both before and after this change.